Add webpage translation functionality#25
Add webpage translation functionality#25SleeplessOne1917 wants to merge 1 commit intoDeepLcom:mainfrom
Conversation
JanEbbing
left a comment
JanEbbing
left a comment
There was a problem hiding this comment.
Thanks so much, this looks great! Just some small nitpicks.
There is also a CHANGELOG.md file where you should mention this.
I can add a test for this later.
I also think there's a slightly cleaner way to enable the HTTPClient to make external HTTP requests, I will think a bit about it.
|
|
||
| private async getContentFromWebpage(webpageUrl: string | URL): Promise<string> { | ||
| const { statusCode, content, contentType } = | ||
| await this.httpClient.sendRequestWithBackoff<string>('GET', webpageUrl.toString()); |
There was a problem hiding this comment.
Actually, using this method will leak the DeepL API key to this website (as it's in the headers).
I think a simple way to solve this is to add a method like sendExternalWebRequest to the httpClient, that just reimplements sending an HTTP request, using e.g. the proxy config from the client and any options added in the request, and use that for the downloadWebpage functionality.
There was a problem hiding this comment.
I ended up stripping the Authorization header from the request before it gets sent if the request is a webpage request. It seemed more straightforward than making a whole new function. See my other comment.
| ); | ||
|
|
||
| if (!isDeepLUrl && axiosRequestConfig.headers) { | ||
| delete axiosRequestConfig.headers.Authorization; |
There was a problem hiding this comment.
This is where I strip the API key if the request is going to an external domain.
|
@JanEbbing I made the requested changes awhile ago. Pinging you in case you didn't see. |
JanEbbing
left a comment
JanEbbing
left a comment
There was a problem hiding this comment.
Manually deleting the API key from the headers seems fine to me. Requesting 2 small changes, then this should be good to go, thanks so much for your work!
|
Awesome, thanks - as this is a bigger feature we are going through an internal review at the moment. To be consistent with the project standards, could you please squash all commits into a single one and use conventional commit naming ( |
|
I can do that, although isn't commit squashing done at merge time? |
|
@JanEbbing |
SleeplessOne1917
force-pushed
the
translate-webpage
branch
from
ba6d044 to
3da0e3d
Compare
|
When will this be merged? It's a great feature! |
|
@JanEbbing Do you plan on merging this at any point? |
3 participants
This is implementing the enhancement from #21. Closes #21.